[NNVM][FRONTEND][Keras] Support for reusing layers #1192
Conversation
|
@Huyuwei, I updated my codes based on your comment. Can you review this? |
nnvm/python/nnvm/frontend/keras.py
Outdated
| @@ -475,7 +475,7 @@ def from_keras(model): | |||
| symtab = SymbolTable() | |||
| for keras_layer in model.layers: | |||
| if isinstance(keras_layer, keras.engine.topology.InputLayer): | |||
| keras_layer.name = 'data' | |||
| keras_layer.name = keras_layer.name | |||
nnvm/python/nnvm/frontend/keras.py
Outdated
| # we append node index to the symbol name to make it unique. | ||
| # The one exception is InputLayer. Changing input variable names after conversion | ||
| # would confuse users, so we should keep them as far as possible. Fortunately, | ||
| # it's safe because all the input layers have unique names in Keras. |
There was a problem hiding this comment.
they are named to input_1, input_2, input_3 ... by default.
nnvm/python/nnvm/frontend/keras.py
Outdated
| if isinstance(pred, keras.engine.topology.InputLayer): | ||
| predecessors.append(pred.name) | ||
| else: | ||
| predecessors.append(pred.name + str(i)) |
There was a problem hiding this comment.
pred.name + '_' +str(i) will be clearer?
There was a problem hiding this comment.
Sounds better, thanks!
There was a problem hiding this comment.
On the second thought, I think pred.name + ':' + str(i) will be better since Keras also adds '_[num]' suffixes to layer names.
nnvm/python/nnvm/frontend/keras.py
Outdated
|
|
||
| # With the current implementation, _convert_merge and _convert_concat | ||
| # expects their inputs to be List. For now, we handle them differently, | ||
| # but in future, we could have a unified way here. |
There was a problem hiding this comment.
@kazum Keras can't create multiple merge or concat layers from the same name instance, is this correct? Seems this is one assumption here.
Another assumption is keras layers other than merge or concat have only one predecessor. I think this is correct. Can you verify it?
There was a problem hiding this comment.
@kazum Keras can't create multiple merge or concat layers from the same name instance, is this correct? Seems this is one assumption here.
I've confirmed it's not correct. I'll add a test case for that and fix the problem.
Another assumption is keras layers other than merge or concat have only one predecessor. I think this is correct. Can you verify it?
Fortunately, looks like the assumption is no longer necessary in the fixed version.
|
@kazum The new implementation is clean and elegant! |
Moved from dmlc/nnvm#501